-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add IBM watsonx AI provider in the list of providers to connect to IBM hosted LLMs #8092
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add IBM watsonx AI provider in the list of providers to connect to IBM hosted LLMs #8092
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I've reviewed the changes and found several issues that need attention before this can be merged. The implementation adds IBM watsonx AI provider support, but there are some critical missing pieces and unrelated changes that should be addressed.
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some issues that need attention. See the inline comments for details.
| } | ||
|
|
||
| this.service = WatsonXAI.newInstance(serviceOptions) | ||
| this.service.getAuthenticator().authenticate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Unawaited authenticator.authenticate() in a non-async constructor risks a race and unhandled rejection. Either remove and rely on lazy auth, await in an async factory, or defer auth to the first API call.
| let embedding = response.result.results[0].embedding | ||
| if (!embedding || embedding.length === 0) { | ||
| console.error(`Empty embedding returned for text at index ${textIndex}`) | ||
| const expectedDimension = this.getExpectedDimension(modelToUse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Generating fallback embeddings (0.0001-filled vectors) pollutes the index and harms retrieval. Prefer surfacing an error and skipping the item (with logging) so quality isn’t degraded.
| detailedMessage = `Connection failed: ${errorMessage}. Please check your network connection and base URL.` | ||
| } | ||
|
|
||
| await vscode.window.showErrorMessage(detailedMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Provider layer shouldn’t call VS Code UI (window.showErrorMessage). Emit an error event/result and let UI handle display to keep layers separated and enable headless usage.
| throw new Error("Base URL is required for IBM Cloud Pak for Data") | ||
| } | ||
|
|
||
| if (username) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] For Cloud Pak, if the username (and corresponding auth) is missing, the client is created without authenticator and the call will fail later. Validate required fields up front and return an error.
|
|
||
| async createEmbeddings(texts: string[], model?: string): Promise<EmbeddingResponse> { | ||
| const MAX_RETRIES = 3 | ||
| const INITIAL_DELAY_MS = 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Hardcoded delays/rate limits (INITIAL_DELAY_MS/REQUEST_DELAY_MS/MAX_CONCURRENT_REQUESTS) will either throttle too much or still trip 429s. Make these configurable and consider adaptive backoff using response headers.
| baseUrl = apiConfiguration.watsonxBaseUrl || "" | ||
| } | ||
|
|
||
| if (platform === "ibmCloud" && (!apiKey || !baseUrl)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Refresh Models for IBM Cloud proceeds without validating projectId; downstream requests that require projectId will fail. Block the action until projectId is provided.
| const platform = this.options.watsonxPlatform | ||
|
|
||
| try { | ||
| const serviceOptions: any = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] serviceOptions typed as any; replace with the SDK’s options type to improve maintainability and compile-time safety.
| password?: string, | ||
| ): Promise<Record<string, ModelInfo>> { | ||
| try { | ||
| let options: any = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] options typed as any here and below; add concrete types for SDK client configuration and response mapping (model spec shape).
| import type { ModelInfo } from "../model.js" | ||
|
|
||
| export type WatsonxAIModelId = keyof typeof watsonxAiModels | ||
| export const watsonxAiDefaultModelId = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] Empty default model id can lead to runtime calls with an empty model id if validation is bypassed. Prefer a safe explicit default (e.g., ibm/granite-3-3-8b-instruct) or require selection.
| defaultModelId={watsonxAiDefaultModelId} | ||
| models={watsonxModels && Object.keys(watsonxModels).length > 0 ? watsonxModels : {}} | ||
| modelIdKey="watsonxModelId" | ||
| serviceName="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] ModelPicker serviceName/serviceUrl are empty; populate to improve UX and consistency with other providers.
Related GitHub Issue
Closes: #8087
Roo Code Task Context (Optional)
Description
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
This PR adds IBM watsonx as a new AI provider, including configuration, validation, UI integration, and testing support.
provider-settings.tsandindex.ts.validate.tsto include validation for IBM watsonx API keys, project ID, and platform-specific settings.CodeIndexPopover.tsxandApiOptions.tsx.ko,nl, andpt-BR.watsonx.spec.tsandembedders/watsonx.spec.ts.This description was created by
for 8e0e30c. You can customize this summary. It will automatically update as commits are pushed.